-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Add 4.08 Grammar #66
base: master
Are you sure you want to change the base?
Conversation
646b4ca
to
06ed613
Compare
1bded7c
to
d7b4ee6
Compare
@ceastlund turns out I already opened a WIP PR! I followed your suggestions ie I added dummy 4.08 modules pointing to the unchanged I went for the dumbest approach when writing the node migration functions as I wasn't sure where I was heading. I only just recently started cleaning it up a bit but it's still fairly simple and we can probably factor a lot of code there. One thing I noticed is that I always passed the same |
Good point about the version arguments. If you like I'll make a PR to remove those arguments. |
For the test failures, it might help if we wrote some tests directly against the History interface. We could then test individual conversions, downgrade and upgrade, against both randomized and hard-coded inputs, and choose the hard-coded ones to stress each AST change. We'd want to test round-tripping, and that output satisfies the right grammar. I think these tests would be easier to use to get the conversions right, and the Ppx_ast tests would be more sanity checks. Want to make a PR adding some tests and I'll take a look at it? |
Yeah I'll try to add such tests! |
No description provided.